-
Notifications
You must be signed in to change notification settings - Fork 477
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feat(core)!: implement an interceptor for the logging layer #4961
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks a lot for this PR!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
After this refactor, the LoggingLayer
is no longer limited to just log
. Users can now employ tracing
or any other logging libraries they prefer. We could potentially offer enhanced support for these use cases by providing default tracing
/log
support.
It's not part of this PR, we can discuss them later.
Hi, I plan to include this PR in our upcoming 0.49.0 release. However, if you are currently busy, we can also postpone it to the next release. Which do you prefer? |
Okay, I'll address the comments in these two days. |
Thanks a lot! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Others look great to me, thanks!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks a lot, mostly LGTM! We only need to get CI happy.
- the haskell test doesn't make sense, we can remove the
testLogger
part
- The unit test of core failed. Please note we can't use
crate
in doc test, we should useopendal::Error
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks a lot!
Which issue does this PR close?
Closes #4918.
Rationale for this change
What changes are included in this PR?
Are there any user-facing changes?
Breaking changes: This PR removes
with_error_level()
,with_failure_level()
andwith_backtrace_output()
from theDefaultLoggingLayer
.It adds a new trait
LoggingInterceptor
to log messages.The
LoggingLayer
has a new method to construct a new layer from an interceptor.